Review from Claude :

Review contextualisée — PR #1085 « feat: Behat extension »

Contexte

PR « méta » (107 fichiers, +6289/−89) regroupant toute la fonctionnalité d'extension
Behat pour Foundry, livrée comme package séparé zenstruck/foundry-behat
(subtree-split depuis src/Test/Behat). Elle introduit un DSL en langage quasi-naturel
(Given there is a contact named A with: …), un registre d'objets nommés, 4 stratégies
de reset DB, le support DAMA, et l'override/traduction des steps. Quelques touches
au cœur de Foundry servent de points d'extension. Auteur : nikophil (= l'utilisateur).
Fixe #1051 et #235.

Review faite contre la constitution (BC, agnosticisme persistance, DX, qualité/tests,
compat PHP 8.1) et les conventions CLAUDE.md. Severité : 🔴 bloquant · 🟠 important ·
🟡 mineur/nit · 🟢 décision validée.

1. Verdict global

Travail de grande qualité, mûr (« 90% ready »), bien testé côté fonctionnel (features
avec une riche couverture des cas d'erreur). La majorité des décisions d'architecture
sont justifiées. Les vrais sujets sont : (a) un couplage cœur→sous-package à assumer,
(b) un pattern service-locator récurrent inter-conteneurs, (c) quelques bugs de bord,
(d) de la dette/TODO résiduels qui ne doivent pas être livrés tels quels, (e) des trous
de tests/CI sur le cœur et la compat PHP. Aucun de ces points n'est rédhibitoire mais
plusieurs doivent être traités avant merge/release.

Sur les 8 commentaires Copilot : 2 sont erronés (hook/listener, voir 3.5 ; et le
"breaking change" sur les enums de test), les autres pointent de vrais sujets mineurs.

2. Décisions d'architecture (« manage les solutions choisies »)

2.1 🟢 Package séparé + subtree-split — VALIDÉ (avec réserves)

Raison invoquée (Behat ne supporte pas SF 8) solide ; isole un cycle de vie/contraintes
différents. Réserves à lever :

• 🟠 "zenstruck/foundry": "dev-behat-extension" dans src/Test/Behat/composer.json
  (déjà flaggé par l'auteur « 🚨 needs to be changed »). Doit devenir ^2.x/^2.7
  avant publication, sinon le package tire une branche de dev.
• 🟠 autoload-dev pointe ../../../tests/Fixture : après le subtree-split vers le
  repo foundry-behat, ce chemin n'existe plus → les tests du package splitté ne sont
  pas exécutables en standalone. Le setup de test est couplé au monorepo
  (symlink-vendor.sh, chemins relatifs). À documenter ou à rendre indépendant.
• 🟡 .env, symfony.lock committés dans le sous-package : ok pour un repo applicatif,
  à confirmer pour un package distribué.

2.2 🟠 Couplage cœur → Behat : ZenstruckFoundryBundle::build() réfère BehatServicesCompilerPass

src/ZenstruckFoundryBundle.php:281 :

if (\class_exists(BehatServicesCompilerPass::class)) {
    $container->addCompilerPass(new BehatServicesCompilerPass()); // @phpstan-ignore argument.type
}

C'est le seul point où le cœur connaît le namespace …\Test\Behat (l'auteur l'assume).
Inversion de dépendance : le cœur dépend (optionnellement) du sous-package.

• Fragilité : si la classe est renommée côté Behat, class_exists devient false
  silencieusement → la feature se désactive sans erreur. Aucun test ne garantit que
  ce câblage s'active réellement.
• Le use en tête + @phpstan-ignore argument.type masquent que le type n'est pas
  analysable depuis le cœur.

Alternatives à arbitrer :

1. Laisser le package Behat livrer un mini-bundle Symfony que l'utilisateur ajoute à son
   test kernel (pureté ↑, DX ↓ : config utilisateur en plus).
2. Garder l'approche actuelle mais : (a) référence par chaîne de classe + check
   is_a(..., CompilerPassInterface::class, true) plutôt que use direct, (b) ajouter
   un test d'intégration vérifiant que le pass est bien enregistré quand le package est là.

Recommandation : option 2 a minima (documenter + test de non-régression du câblage).

2.3 🟠 Service-locator inter-conteneurs (pattern récurrent)

FoundryCallFilter, DatabaseResetListener, BootConfigurationListener,
LoadFixturesListener récupèrent leurs dépendances via
$symfonyKernel->getContainer()->get('.zenstruck_foundry.…') — services privés (préfixe
.) rendus public() dans config/behat.php, le tout saupoudré de @phpstan-ignore.
Justifié (pont entre le conteneur Behat et le conteneur du kernel Symfony booté), mais :

• multiplie les get() non typés et les ignores PHPStan ;
• rend public des services qui n'ont pas vocation à l'être.

Reco : encapsuler le pont dans un seul objet accessor (ex. FoundryServices
prenant le kernel et exposant des getters typés factoryResolver(): …,
objectRegistry(): …, fixtureStoryResolver(): …). On centralise les casts/ignores en
un point, on supprime la répétition, on documente l'intention.

2.4 🟢 Point d'extension cœur via événement StateAddedToStory — ACCEPTABLE

Story::addState() dispatch un event quand un objet est ajouté à l'état d'une story,
gardé par hasEventDispatcher() (no-op hors Symfony). Couplage propre via événement,
altitude correcte. Réserves :

• 🟡 is_object($value) est large (Copilot) — pour le besoin (nommer les objets de story)
  c'est volontaire et acceptable ; la suggestion Copilot instanceof Proxy serait trop
  restrictive (objets non-proxy/InMemory exclus). Ne pas suivre Copilot ici.
• 🟡 StateAddedToStory déclare @template T of object non utilisé (la prop est
  public readonly object $object). Supprimer le template ou typer réellement.

2.5 🟢 foundry.hook → kernel.event_listener (config/persistence.php) — VALIDÉ (Copilot a tort)

AfterPersist est dispatché par le vrai EventDispatcher Symfony
(PersistentObjectFactory:574). Le tag foundry.hook n'était qu'un wrapper
HookListenerFilter filtrant par classe ; avec class => null, __invoke() est un
passe-plat pur. Le passage en kernel.event_listener direct est donc strictement
équivalent (une indirection en moins). La suggestion Copilot de garder les deux tags
provoquerait un double dispatch du tracker → bug. Décision de l'auteur correcte ;
le système de hooks par-factory (#[AsFoundryHook]) reste intact.

2.6 🟢 getIdentifierValues(): mixed → array<string,mixed> — VALIDÉ (amélioration)

Sur 2.x, PersistenceManager était typé mixed mais déléguait déjà à des stratégies
(PersistenceStrategy, ORM, Mongo) toutes typées array. Le resserrement aligne le
type sur la réalité (covariant, sûr), classe @internal. Le @var de
PersistedObjectsTracker est mis en cohérence. Bonne hygiène.

2.7 🟠 PersistenceManager final retiré pour le mock — SMELL à corriger

@final (docblock) + classe non final = contradiction (Copilot a raison).
Motif réel (commentaire auteur) : « je mocke cette classe dans des tests ». Or
ObjectRegistry n'a besoin que de getIdentifierValues(object): array.
Reco : extraire une interface étroite (ex. IdentifierExtractor) implémentée par
PersistenceManager, l'injecter dans ObjectRegistry, et rétablir final sur
PersistenceManager. On préserve l'invariant d'immuabilité/finalité du cœur et on mocke
une abstraction plutôt qu'un orchestrateur concret.

3. Bugs & correctness

3.1 🟡 Code mort dans LoadFixturesCommand (Copilot, confirmé)

Après $stories = $this->fixtureStoryResolver->resolve($name), le bloc
if (!$stories) throw InvalidArgumentException est inatteignable : resolve() lance
FixtureStoryNotFound ou renvoie un tableau non vide. À supprimer (le test
it_throws_if_story_does_not_exist attend déjà FixtureStoryNotFound).

3.2 🟡 Double résolution dans LoadFixturesCommand

resolve($name) puis hasFixture($name) (re-lookup) pour choisir le message. Mineur ;
faire renvoyer à resolve() un objet portant le type (fixture vs group) éviterait le
second lookup.

3.3 🟡 FixtureStoryResolver : garde redondante / chemin mort

resolveGroup() re-teste isset($groupedStories[$groupName]) alors que resolve() a déjà
appelé hasGroup() ; FixtureStoryNotFound::forGroup() est donc inatteignable via
resolve(). Simplifier ou couvrir explicitement par un test.

3.4 🟠 Coercion d'enum numérique (FoundryCallFilter:181)

$value = \is_numeric($value) ? (int) $value : $value;
return $expectedTypeClass::tryFrom($value) ?? throw …;

Suppose que « numérique ⇒ enum int-backed ». Un enum string-backed dont les valeurs
sont des chaînes numériques ('0', '1') verrait tryFrom((int)"1") échouer.
Reco : déterminer le backing type réel de l'enum (reflection sur le type du
BackedEnum) avant de caster, ou ne caster en int que si l'enum est int-backed.

3.5 🟡 catch (\Throwable) pour DateTime trop large (Copilot partiellement faux)

Le @phpstan-ignore catch.neverThrown est nécessaire (PHPStan pense que le ctor ne
jette pas) — la suggestion Copilot de retirer l'ignore casserait PHPStan. En revanche le
catch (\Throwable) devrait être catch (\Exception) /
\DateMalformedStringException (PHP 8.3+) pour ne pas avaler des erreurs inattendues.

3.6 🟡 Précision de comparaison de dates (FoundryContext:114-117)

Comparaison au format Y-m-d H:i:s : ignore les microsecondes. Peut masquer une
différence sub-seconde (faux positif d'égalité). Acceptable pour des fixtures, à
documenter ou rendre configurable.

3.7 🟠 FoundryTableNode : hack sur les internes de Behat

newInstanceWithoutConstructor() + set($node, 'table', …) pour contourner le contrôle
« scalaires uniquement » du constructeur de TableNode, plus lecture de la prop
privée maxLineLength via \Closure::bind(...) dans FoundryCallFilter:93. Couplage
fort aux internes privés de Behat → casse silencieuse possible à une montée de version
Behat. Le commentaire « super hacky but seems to work » l'admet. À encadrer par un test
de garde + commentaire pointant la version Behat ciblée.

3.8 🟡 PHPDoc @method getRowsHash() dupliqué (FoundryTableNode:27-28)

Deux déclarations @method … getRowsHash() avec types de retour contradictoires
(array<string, string|list<mixed>> puis list<mixed>). Copier-coller à nettoyer.

3.9 🟠 Méthodes has*() qui lancent des exceptions (DatabaseResetListener)

hasResetDbTag() / hasNoResetDbTag() renvoient un bool mais lancent
InvalidResetDbTag/DamaNativeExtensionIncompatibility selon le mode. Viole
command-query separation : un prédicat nommé hasX() ne devrait pas avoir d'effet de
bord/échouer. Rend la logique difficile à suivre. Reco : séparer la validation
(méthodes assertTagsAreValid() appelées explicitement dans validateScenario/
validateFeature) de la lecture (hasResetDbTag() pur).

3.10 🟠 Cycle de vie distribué sur 3 listeners

Le boot/shutdown de Configuration et le reset() du ObjectRegistry sont éclatés entre
BootConfigurationListener (after feature/exercise) et DatabaseResetListener (after
scenario en mode SCENARIO, + StoryRegistry::reset()), avec priorités numériques
(100, -10, -100). Difficile à raisonner, risque de double-reset/ordre. Reco :
documenter la matrice (mode × event × action) dans un seul endroit, idéalement
centraliser la responsabilité de cycle de vie, et couvrir les transitions par des tests.

4. Dette technique / résidus à ne pas livrer

4.1 🟠 NoPersistenceObjectsAutoCompleter — fuite de chemins repo dans du code livré

src/Maker/Factory/NoPersistenceObjectsAutoCompleter.php :

if (\str_contains($phpFile->getRealPath(), 'var/cache')
    || \str_contains($phpFile->getRealPath(), 'src/Test/Behat')) { continue; }

• var/cache : skip générique, ok.
• src/Test/Behat : spécifique au repo Foundry, livré aux utilisateurs (code mort
  chez eux, et fuite de structure interne). À retirer du code livré (ou rendre les
  exclusions configurables).
• Le todo: ⚠️ this class is problematic and should be at least refactored or removed
  est aussi livré aux utilisateurs. À traiter ou au moins ne pas embarquer le ⚠️.

4.2 🟡 composer.json (principal) : suppression de post-install-cmd

Le scripts.post-install-cmd (@composer bin phpstan install, … phpbench install) est
retiré — possible régression DX pour les contributeurs (outils plus auto-installés après
composer install). Vérifier que c'est intentionnel et que la CI installe ces outils
explicitement (la CI Behat fait composer bin phpstan install, mais le reste ?).

4.3 🟡 Scope creep

La PR embarque des changements non strictement Behat : refactor enums de test
(SomeEnum→IntBackedEnum + StringBackedEnum), réorganisation configureRoutes,
signature configureContainer, bin/console. Cohérents avec le besoin (fixtures de test
enrichies), mais à signaler pour la relecture.

5. Tests & qualité (Constitution V)

5.1 🟠 FixtureStoryResolver : aucun test unitaire

Nouvelle classe cœur avec branchements (resolve/hasFixture/hasGroup/exceptions),
couverte seulement indirectement par LoadFixturesCommandTest. Ajouter un test
unitaire dédié (cas : fixture, groupe, inconnu, collisions, listes vides).

5.2 🟡 StateAddedToStory : pas de test cœur du dispatch

Le dispatch dans Story::addState() n'est vérifié que par les features Behat. Un test
cœur (event dispatché ss! objet + dispatcher présent) sécuriserait ce point d'extension.

5.3 🟠 Incohérence compat PHP : ^8.1 déclaré, CI teste 8.3/8.5

src/Test/Behat/composer.json requiert php: ^8.1 mais .github/workflows/behat.yml
ne teste que 8.3 et 8.5. La constitution exige la compat PHP 8.1 (avec polyfills).
Soit ajouter 8.1/8.2 à la matrice, soit remonter la contrainte à ^8.3. En l'état la
promesse ^8.1 n'est pas vérifiée (et array_first/array_last ne sont garantis que via
polyfill-php85, bien requis — ok, mais non testé sur 8.1).

5.4 🟢 Points positifs

• ObjectRegistryTest couvre les cas durs (clé composite → exception, UUID, type d'id
  invalide).
• Les .feature couvrent abondamment les cas d'erreur (scénarios (!)).
• ResetDatabaseTestCase : workaround --skip-mapping pour doctrine/orm < 3 bien
  commenté.

6. DX / design d'API (retours relecteurs)

• 🟠 jdeniau — généricité des steps : there is a(n) :x, :x named :y sont très
  génériques et peuvent entrer en collision avec d'autres contextes Behat. Choix DX
  assumé (langage naturel) mais à arbitrer : préfixe optionnel / documentation du risque
  d'override (le mécanisme steps/translations permet justement de réécrire).
• 🟡 jdeniau — convention « ligne finissant par : avant un tableau » : respectée via
  with:. Étudier les variantes named/called/a(n) proposées pour la robustesse.
• 🟡 Zoo de tokens magiques : <ref(t,n)>, <id(t,n)>, <lastId>, <lastId(t)>,
  colonne _ref. Beaucoup de syntaxes ; s'assurer qu'elles sont toutes documentées et
  cohérentes. Incohérence DX : le singulier nomme via named X (arg de step), le pluriel
  via colonne _ref — deux mécanismes pour la même intention.
• 🟡 Pluralisation anglais-only (EnglishInflector) ; mitigée par l'attribut
  #[FactoryShortName(short, plural)] et les traductions. OK à documenter.
• 🟢 Doc fournie et conséquente (docs/index.rst ~2730-3125), l'auteur a investi.

7. Synthèse des actions recommandées (par priorité)

Avant merge

1. Trous de tests/CI : test unitaire FixtureStoryResolver (5.1) ; aligner compat PHP
   déclarée vs testée (5.3).
2. Résidus livrés : sortir le skip src/Test/Behat + le todo ⚠️ du Maker (4.1) ;
   confirmer la suppression post-install-cmd (4.2).
3. PersistenceManager : interface étroite + restaurer final (2.7).
4. Bug enum numérique string-backed (3.4).

Avant release du package
5. dev-behat-extension → contrainte stable (2.1) ; rendre les tests du package
splittable (autoload-dev) (2.1).
6. Documenter/tester le câblage cœur→Behat (2.2).

Qualité / dette (souhaitable)
7. Accessor unique pour le pont inter-conteneurs (2.3).
8. Séparer validation/lecture dans DatabaseResetListener (3.9) ; clarifier le cycle de
vie (3.10).
9. Nettoyages : code mort LoadFixturesCommand (3.1) ; @template inutile (2.4) ;
PHPDoc dupliqué (3.8) ; catch (\Throwable) (3.5) ; garde du hack FoundryTableNode
(3.7).

À ne PAS suivre dans les retours Copilot : garder les deux tags hook/listener (2.5,
double dispatch) ; instanceof Proxy au lieu de is_object (2.4, trop restrictif) ;
« breaking change » sur les enums de fixtures de test (faux : non public) ; retirer le
@phpstan-ignore du catch DateTime (3.5, casserait PHPStan).

8. Vérification (si des correctifs sont appliqués)

Chaîne complète tests() (.spells), en ciblant les zones touchées :

• bin/tools/phpstan/vendor/phpstan/phpstan/phpstan analyse (cœur) et
  … -c src/Test/Behat/phpstan.neon (sous-package).
• ./phpunit --exclude-group maker (dont LoadFixturesCommandTest + nouveau test
  FixtureStoryResolver).
• src/Test/Behat/vendor/bin/phpunit -c src/Test/Behat/phpunit.dist.xml.
• behat-all (tous profils : scenario, no-dama, native-dama, manual, manual-dama,
  feature, feature-dama, disabled, override-steps).
• Pour 5.3 : étendre la matrice CI à PHP 8.1/8.2 (ou ajuster la contrainte) et relancer.